code-review-graph review

Overall risk: 0.50 (MEDIUM) — 22 changed function(s)/class(es), 0 affected flow(s), 4 test gap(s) (3 with no tested caller found, 1 reached only through a caller)

Risk-scored changes

Test gaps

• code_review_graph/daemon.py::_WindowsJob.IoCounters (code_review_graph/daemon.py:139)
• code_review_graph/daemon.py::_WindowsJob.BasicLimitInformation (code_review_graph/daemon.py:142)
• code_review_graph/daemon.py::_WindowsJob.ExtendedLimitInformation (code_review_graph/daemon.py:155)

Reached only through a caller

No test names these directly, but a tested caller reaches them along the call path shown. That is a path in the call graph, not a record of execution -- the caller's tests may never take this branch. Treat it as where to look, not as coverage: these count as gaps and are scored as untested.

• code_review_graph/daemon.py::WatchDaemon._start_watcher (code_review_graph/daemon.py:1634) — via code_review_graph/daemon.py::WatchDaemon._check_health, 1 hop(s)

Token savings: this graph-backed report used ~28,859 fewer tokens (~88%) than reading every changed file in full (estimated, chars/4 approximation).

Powered by code-review-graph — local-first analysis; no code leaves the CI runner.

Blocked: the Windows Job Object lifecycle is unverified, current Windows/type checks fail, and successful signal delivery is still treated as proof that the daemon stopped. In an isolated unit test, simulate a recorded running daemon and reproduce the stop defect by making os.kill return successfully while daemon.pid_alive remains true, then invoke the daemon stop command; this PR clears the PID and reports success without establishing termination. Retain the PID until death is confirmed and verify parent termination and child cleanup with real Windows processes.

Blocked: the Windows Job Object lifecycle is unverified, current Windows/type checks fail, and successful signal delivery is still treated as proof that the daemon stopped. In an isolated unit test, simulate a recorded running daemon and reproduce the stop defect by making os.kill return successfully while daemon.pid_alive remains true, then invoke the daemon stop command; this PR clears the PID and reports success without establishing termination. Retain the PID until death is confirmed and verify parent termination and child cleanup with real Windows processes.

For sure @tirth8205 I will take up the tests and update you shortly!!

Yes @tirth8205 I just got your point and yet got to know that just os.kill() execution doesn't retrieve that daemon is stopped successfully correct me if I got wrong somewhere ..

-> 135 daemon/watch/process tests passed, 2 skipped on Linux.
mypy, ruff, and git diff --check passed.
->The Windows Job Object integration tests could not be run locally because the current environment is Linux; they will need to be verified by Windows CI.

The portable stop correction from d651370 is integrated on integration/token-efficiency-hardening as 660b7ac, with nine additional tests for exit checks, PID retention and restart refusal. The Windows Job Object portion of #925 remains blocked: native lifecycle verification is missing and the current Windows CI job fails. Its new child test generates invalid Python (...; while ...); add compile(parent_code, "<parent>", "exec") immediately before Popen in test_windows_job_close_terminates_watcher_children to reproduce the SyntaxError without launching a process.

The portable stop correction from d651370 is integrated on integration/token-efficiency-hardening as 660b7ac, with nine additional tests for exit checks, PID retention and restart refusal. The Windows Job Object portion of #925 remains blocked: native lifecycle verification is missing and the current Windows CI job fails. Its new child test generates invalid Python (...; while ...); add compile(parent_code, "<parent>", "exec") immediately before Popen in test_windows_job_close_terminates_watcher_children to reproduce the SyntaxError without launching a process.

yeah @tirth8205 finally all the test passed including windows daemon!!

Checks pass and the code looks right. Missing: tests for the edge cases it introduces.

• tests/test_daemon.py (TestWatchDaemon, cross-platform, mock job): set daemon._windows_job = MagicMock() with job.assign.side_effect = OSError(...) and mock_proc.poll.return_value = None; assert mock_proc.terminate() and mock_proc.wait() were called, the alias is absent from daemon._children, and the...
• Same test family: make mock_proc.wait raise subprocess.TimeoutExpired on the first call so the proc.kill() branch is exercised; assert kill() was called.
• Variant: job.assign raises but mock_proc.poll() returns a non-None returncode (child died before assignment) -> assert terminate() NOT called and alias not tracked.
• tests/test_daemon.py: patch WatchDaemon._terminate_child with side_effect=RuntimeError, set daemon._windows_job = MagicMock(), call daemon.stop() inside pytest.raises; assert job.close.assert_called_once().
• Either guard the second proc.wait(timeout=5) after proc.kill() in _start_watcher (daemon.py ~1336) so TimeoutExpired cannot escape and leak log_fd / kill the health thread, and add a test for it , or document why it cannot time out.

Every new branch in the code should have a test, including the failure branch. Disagree with any of these? Say so here first.

PRs now target staging, not main. Yours was retargeted already, so nothing to do there.

f6d5bcf does all five, including the fifth, which you fixed rather than documented around. Good call.

Gates on c86fe2e merged into staging 9c13f4c: 3341 passed, 11 skipped, 2 xpassed, ruff clean on code_review_graph/, mypy clean. The five new tests all pass cross-platform here on macOS. The only two skips in that area are the pre-existing native Windows Job Object integration tests, and Windows daemon and file handles is green on CI at 9m48s, so the Windows job that was failing is no longer failing.

I mutation-checked the fifth item because it is the one that matters most. Reverting the try/except around the second proc.wait(timeout=5) in _start_watcher makes test_start_watcher_assignment_failure_handles_kill_wait_timeout fail with the TimeoutExpired escaping, and nothing else fails. That is a real guard, not a test that passes either way.

On the mypy trouble in your screenshot: mypy is clean here with the exact gate command, uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional. If you saw errors, you were almost certainly pointing mypy at tests/ as well, which is not part of the gate and is not type-clean. Nothing for you to fix.

One nit, not blocking. tests/test_daemon.py:614 is 101 characters. CI only lints code_review_graph/, so it will not fail, but the repo convention is 100. Wrap it if you push again for any other reason.

Queued for merge.

Removing needs-tests.

The cleanup inside except Exception: in _start_watcher is itself unguarded, so on Windows one assign failure leaves the daemon permanently dead. proc.terminate() and proc.kill() are called bare, and log_fd.close() sits after them. TerminateProcess can fail with ERROR_ACCESS_DENIED against a process that is already exiting, and Popen.terminate() raises OSError when it does.

Repro on the head: set daemon._windows_job to a stub whose assign() raises OSError, hand _start_watcher a process object wrapping a real child whose terminate() raises PermissionError, then call it.

head 9abf853: escaped=PermissionError: Access is denied  child_still_alive=True  fd_leak=1
base ca23a73: escaped=None                               fd_leak=0

Three things break at once. The original assign error is swallowed and replaced, the log handle leaks, and the exception escapes _start_watcher into _check_health. _health_loop calls _check_health with no exception guard, so the health thread dies silently and no watcher is restarted again for the life of the daemon. Nothing covers this path: grep -n 'terminate.side_effect\|kill.side_effect' tests/test_daemon.py returns nothing, and only wait raising TimeoutExpired is exercised. About four lines fix it: except OSError around the terminate/kill block, and log_fd closed in a finally.

All five items from the last round are in the head and each one is individually mutation-covered. Four reverts were run, including dropping only the inner guard around the post-kill proc.wait(timeout=5), and every one failed a test. The ctypes layout matches the Win32 x64 ABI exactly at 144/64/48 bytes with offsets 16/64/112, info class 9 and flag 0x2000 are right, and the job handle is non-inheritable so kill-on-close can fire. Delta against staging is exactly +6 passed and +2 skipped.

tests/test_daemon.py:614 is still 101 characters. CI only lints code_review_graph/ so it will not fail, but I asked for it last round.

Three Windows-only items, none blocking:

• _initial_build runs the build through subprocess.run and never assigns it to the job, so a daemon killed during a fresh-repo build orphans that process. Same in reconcile() for a newly added repo.
• tests/test_watch_robustness.py constructs WatchDaemon bare at lines 1346, 1396, 1427 and 1468 and never closes the job. That file runs in the windows-native CI job and _WindowsJob has no finalizer, so each one leaks a handle. The daemon_env fixture in test_daemon.py got the _windows_job = None treatment and these did not.
• Assignment happens after Popen returns, so a grandchild spawned in that window escapes the job for good. subprocess exposes no thread handle, so CREATE_SUSPENDED is not an option. The docstring should say the job almost always owns them rather than that Windows cannot orphan them.

Mergeable once the terminate/kill block is guarded with a test that sets terminate.side_effect, and line 614 is wrapped.

Thank you for this PR. I tested it on top of the latest staging.

Before I can merge it, please fix these:

1. In _start_watcher, the cleanup after a failed assign() has no guard. This is at code_review_graph/daemon.py:1675-1690. Example: assign() fails, then terminate() raises PermissionError. The error escapes. The log file stays open. The health thread then stops for good. Please catch OSError there. Please close log_fd in a finally. Please add a test with terminate.side_effect and kill.side_effect.
2. tests/test_daemon.py:614 is 101 characters. The limit is 100. Please wrap it.
3. The PR text says Closes #925. crg-daemon stop on Windows terminates the daemon without stopping its watchers, leaving orphans that duplicate on the next start #925 also covers the health file left by a killed watcher. This PR does not change that part. Please change it to Refs #925.

After you push the fix, I will test it again.

I've addressed the requested changes:

• Hardened watcher startup cleanup against OSErrors, ensured kill() is attempted when appropriate, and moved log handle closure into a finally block.
• Added a regression test covering terminate() and kill() failures and health-loop continuation.
• Fixed the test definition's line length.
• Confirmed the PR description already uses Refs #925.

Validation: 76 tests passed, 2 skipped. Ruff lint, the line-length check, and git diff --check passed. ruff format --check still reports pre-existing formatting differences in unrelated lines, which I left unchanged.

I've pushed the fix for re-review.